PR Review: Fix reusable GitHub housekeeping action

Overall this is a focused, well-reasoned bug fix. The path corrections are clearly necessary, the regression test is a good addition, and the README improvements are helpful.

Correctness — Path depth fix ✅

The core fix (../.. → ../../..) is correct. The action lives at .github/actions/github-housekeeping/, so reaching repo root requires going up three levels, not two. The original code would have landed in .github/ which would silently produce wrong build paths. Applied consistently across action.yml (global-json-file), action.yml (build step), and Invoke-PowerForgeHousekeeping.ps1.

Code quality — AddRange → foreach loop

Invoke-PowerForgeHousekeeping.ps1 lines 112–119

The foreach loop with explicit [string] casts fixes a real PowerShell quirk where List<string>.AddRange() rejects an untyped @(...) array. The fix works, but a slightly more concise alternative that avoids the loop entirely would be:

$arguments.AddRange([string[]]@(
    'run', '--project', $project, '--framework', 'net10.0', '-c', 'Release', '--no-build', '--',
    'github', 'housekeeping',
    '--config', $configPath
))

Either approach is fine — the foreach form is arguably more readable for maintainers unfamiliar with the quirk.

Potential fragility — hardcoded net10.0

Invoke-PowerForgeHousekeeping.ps1 line 113

--framework net10.0 resolves the multi-TFM ambiguity correctly, but it is hardcoded. If the CLI project ever drops net10.0 or the repo migrates to a newer TFM, this will silently break. Consider reading the target framework from global.json or a shared variable, or at minimum leaving a comment that this must stay in sync with PowerForge.Cli.csproj.

Test — minor redundancy

GitHubHousekeepingActionTests.cs lines 32–33

Assert.Contains("../../../global.json", actionYaml, StringComparison.Ordinal);
Assert.Contains("../../..", actionYaml, StringComparison.Ordinal);  // redundant

The second assertion is always satisfied when the first passes since ../../../global.json is a superset of ../../... You could drop the second check, or replace it with a more specific assertion (e.g. the $repoRoot path pattern in the build step).

Test — FindRepoRoot heuristic

GitHubHousekeepingActionTests.cs lines 36–44

Walking up and searching for PowerForge/PowerForge.csproj works for the current structure. A .git directory marker is more conventional and resilient to project renames, though either approach is fine for a developer-local regression test.

Documentation ✅

The README additions are clear and genuinely useful:

• The powerforge-ref: main example matches what users actually need.
• The immutable pinning note is good operational guidance.
• The minimal config example is well-structured; the inline comments via $schema point to the right place.

Summary

No blocking issues. The hardcoded framework and the redundant assertion are worth a follow-up but do not prevent merging.

PR Review: Fix reusable GitHub housekeeping action. The core bug fixes look correct and well-scoped. See detailed feedback below.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid set of targeted fixes. The root cause (wrong relative depth) is correctly identified and consistently fixed in both action.yml and the PS1 script.

Correctness

Path depth fix - The depth correction from ../.. to ../../.. is correct. From .github/actions/github-housekeeping/, three levels are needed to reach the repo root. The fix is consistent across action.yml and Invoke-PowerForgeHousekeeping.ps1.

AddRange to foreach fix - This is the right fix. PowerShell array literals produce object[], which is not assignable to IEnumerable[string] in a .NET generic context, so List[string].AddRange() would throw at runtime. The typed [string] cast in the foreach loop is correct.

Potential Issues

1. Hardcoded net10.0 framework string: The framework string is now duplicated across global.json, the .csproj, and the PS1 script. If the project ever targets net11.0, this must be updated manually in the PS1 as well. Consider deriving the TFM dynamically from global.json, or at minimum add a comment noting why it is hardcoded and where else it must be kept in sync.

2. Implicit coupling between build step and --framework: The action.yml Build step compiles the CLI, and the PS1 then runs with --no-build. With --framework net10.0 now explicit, if the build step ever changes target frameworks, running dotnet with --no-build and the old framework flag will fail with a confusing error. The dependency is currently implicit - consider making the framework an action input or adding a comment documenting the coupling.

Test

The magic number 12 in FindRepoRoot() works in practice but is arbitrary. A more robust stop condition would compare against Path.GetPathRoot(current.FullName) to stop at the filesystem root rather than after a fixed number of iterations.

The test itself is a reasonable regression guard - it confirms the relative depth resolves to real files in the actual repo layout, directly validating the fixed paths. Good addition.

Documentation

The README additions are helpful. The minimal JSON config example and the SHA pinning guidance are both valuable for new adopters. Adding powerforge-ref: main makes the reusable workflow example self-contained.

Summary: The core bug fixes are correct and consistent. The main thing worth addressing before merge is the hardcoded net10.0 string - even a comment noting where it must stay in sync would reduce future maintenance risk.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

test comment from agent - pending file write permission

test2

test line with ## heading

short test

short test with -e flag

line1 line2 line3

PR Review - full content test single line

PR Review - Updated

test base64

PR Review

Overall this is a well-structured improvement. The path-depth bug fix is clear and the richer reporting output is a good addition. A few issues worth addressing before merge:

Bugs / Correctness

1. Format-GiB called without null guard on sizeInBytes (Add-ItemDetails)

In both the artifacts and caches branches, items are rendered with Format-GiB ([long]$item.sizeInBytes). If sizeInBytes is null or missing in the JSON response, [long]$null coerces to 0 and produces misleading output (0.0 GiB) instead of -. Format-NullableGiB already exists and handles the null case -- use it here consistently.

2. Write-GitHubOutput multi-line value handling

The simple name=value format used in Write-GitHubOutput only works correctly for single-line values. GitHub's documented format for multi-line values requires a heredoc-style delimiter. File paths are almost never multi-line, but a comment documenting this constraint would prevent misuse if the function is reused for other outputs.

Design / Quality

3. Add-SectionTable has a hardcoded header

The function is named generically (Add-SectionTable) but always emits ## Storage Summary. Either rename it (e.g., Add-StorageSummaryTable) or add a [string] $Title parameter to make it genuinely reusable.

4. Duplicated $deleteState logic in Add-ItemDetails

The artifacts and caches branches both inline identical delete-state resolution logic. Extracting it into a small helper (e.g., Get-DeleteStateLabel) would prevent drift if the logic needs to change.

5. Escape-MarkdownCell uses an unapproved PowerShell verb

Escape is not in the approved PowerShell verb list. Since this is script-internal it won't cause runtime warnings, but ConvertTo-MarkdownCell would be more idiomatic.

Tests

6. Path assertions in GitHubHousekeepingActionTests are substring-only

These assertions check only that ../../.. appears anywhere in the file. Consider asserting the specific known patterns to catch regressions more precisely.

7. FindRepoRoot couples test discovery to a specific project file path

Using PowerForge/PowerForge.csproj as a root marker means renaming that project would silently break test discovery. Not a blocker, just worth noting.

Looks Good

• The ../../.. depth fix is correct -- the action lives at .github/actions/github-housekeeping/, so three levels up reaches the repo root.
• Adding --framework net10.0 to dotnet run with --no-build is sound: the prior build step runs dotnet build without a framework filter, which builds all targets.
• The compatibility alias in reusable-github-housekeeping.yml delegating to powerforge-github-housekeeping.yml is clean.
• The Resolve-WorkspacePath generalization of Resolve-ConfigPath is a nice DRY improvement.

Summary: Items 1 and 3 are the most impactful to fix before merge. Items 2 and 4 are low-risk cleanup. Items 5-7 are minor style/robustness notes.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid improvement — moving the report generation into the C# CLI and fixing the incorrect ../.. depth is the right call. A few issues worth addressing before merge.

🔴 Critical: $args shadows a PowerShell automatic variable

In action.yml, the inline run script does:

$args = @('run', '--project', $project, ...)
& dotnet @args

$args is a read-only automatic variable in PowerShell representing the unbound arguments passed to a script or function. Assigning to it at script scope will either silently fail (the real $args wins) or throw a parse error depending on the PowerShell version. The splatted invocation & dotnet @args may call dotnet with zero arguments.

Use a different name — e.g. $dotnetArgs or $cliArgs:

$dotnetArgs = @('run', '--project', $project, '-c', 'Release', '--no-build', '--framework', 'net10.0', '--', 'github', 'housekeeping', '--config', $env:INPUT_CONFIG_PATH)
if ($env:INPUT_APPLY -eq 'true') { $dotnetArgs += '--apply' } else { $dotnetArgs += '--dry-run' }
& dotnet @dotnetArgs

🟡 Missing config-path default / validation

The old Invoke-PowerForgeHousekeeping.ps1 defaulted $configPath to .powerforge/github-housekeeping.json when INPUT_CONFIG_PATH was empty, and threw if the file didn't exist. The new inline script passes $env:INPUT_CONFIG_PATH directly to --config without a default or an existence check. If a caller omits config-path, the CLI receives --config "" which will likely produce a confusing error message.

Since config-path already has a default in action.yml, this shouldn't happen in practice, but the old defensive check was still a useful guard.

🟡 Token env-var name change is a silent breaking change

The env block changed from:

POWERFORGE_GITHUB_TOKEN: ${{ inputs['github-token'] != '' && inputs['github-token'] || github.token }}

to:

GITHUB_TOKEN: ${{ inputs['github-token'] != '' && inputs['github-token'] || github.token }}

If the CLI was looking for POWERFORGE_GITHUB_TOKEN and now needs GITHUB_TOKEN, callers that relied on the old documented variable name will silently use the wrong token. Worth a note in the PR description / CHANGELOG, and ideally the old name should be kept as an alias in the CLI for at least one release cycle.

🟡 Fragile test assertion on YAML content

In GitHubHousekeepingActionTests.cs:

Assert.Contains("PowerForge GitHub Housekeeping", actionYaml, StringComparison.Ordinal);

This assertion tests the literal content of action.yml but the string "PowerForge GitHub Housekeeping" doesn't appear in the action definition itself (it lives in the workflow name and the Markdown report title). This will fail if the action's name: field doesn't exactly match. Consider either testing the actual action name: field via a YAML parser or dropping this assertion in favour of the more meaningful file-existence checks already present.

🟢 Positive observations

• Moving WriteGitHubHousekeepingOutputs into the C# CLI is the right architecture — it eliminates the fragile JSON-piping-through-PowerShell pattern and makes the output logic testable.
• GitHubHousekeepingReportService is well-structured, the EscapeCell helper correctly sanitises pipe characters and newlines for Markdown tables, and AppendTruncationNotice protects against huge summaries.
• if: always() on the artifact upload step is correct — reports should be uploaded even on failure.
• Adding --framework net10.0 to the dotnet run invocation avoids the ambiguous multi-target error.
• The new regression test for asset path resolution is a good addition; the FindRepoRoot walk-up loop is a clean approach.

Minor nits

• AppendTruncationNotice has the limit 20 duplicated in AppendArtifactTable, AppendCacheTable, and AppendRunnerDetails. A private constant would make it easier to change.
• GitHubHousekeepingReport.Command is hardcoded to "github.housekeeping" but the property has a set; accessor — consider making it init; or private set; since nothing should override the command identifier after construction.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid improvement — the path-resolution bug fix is correct, the move to structured reports is well-designed, and the test coverage is a nice addition. I found a few issues ranging from a potential silent bug to minor nits.

Bugs / Correctness

1. $args is a PowerShell automatic variable (medium-high risk)

In action.yml, the inline run block uses:

$args = @('run', '--project', $project, ...)
& dotnet @args

$args is a reserved automatic variable in PowerShell — it holds the unbound arguments passed to the enclosing script or function. Assigning to it at script scope can behave unexpectedly across PowerShell versions and hosts. The deleted Invoke-PowerForgeHousekeeping.ps1 correctly used $arguments (a List[string]). Consider renaming to $cliArgs or $arguments.

2. --token-env argument is no longer passed to the CLI

The old script explicitly did:

$null = $arguments.Add('--token-env')
$null = $arguments.Add('POWERFORGE_GITHUB_TOKEN')

The new inline script does not pass --token-env GITHUB_TOKEN at all. The token env var is renamed to GITHUB_TOKEN, but if the CLI requires --token-env to know which variable to read, the token will silently be ignored. This would cause all API calls to fail with auth errors at runtime. Please verify the CLI has a fallback that auto-reads GITHUB_TOKEN, or add if ($env:GITHUB_TOKEN) { $cliArgs += '--token-env', 'GITHUB_TOKEN' }.

3. Missing $ErrorActionPreference = 'Stop' and exit-code check

The old .ps1 had $ErrorActionPreference = 'Stop' at the top. The new inline block has neither that nor a check on $LASTEXITCODE. If dotnet run fails (e.g. build error, config missing), the step may continue and report success to the workflow engine. At minimum, add:

$ErrorActionPreference = 'Stop'
# ... run dotnet ...
if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }

4. Config-file existence check removed

The old script validated the config before running:

if (-not (Test-Path -LiteralPath $configPath)) {
    throw "Housekeeping config not found: $configPath"
}

Without this, a missing config file produces a generic CLI error rather than a clear, actionable message. Consider re-adding this guard.

5. Wrong filename in regression test assertion

In GitHubHousekeepingActionTests.cs line 512:

Assert.DoesNotContain("Invoke-GitHubHousekeeping.ps1", actionYaml, StringComparison.Ordinal);

The deleted file was named Invoke-PowerForgeHousekeeping.ps1, not Invoke-GitHubHousekeeping.ps1. This assertion passes trivially (the wrong name was never in action.yml) and provides no regression value. Change to:

Assert.DoesNotContain("Invoke-PowerForgeHousekeeping.ps1", actionYaml, StringComparison.Ordinal);

Behavioral Change (breaking for existing callers)

6. apply logic change in github-housekeeping.yml

Old:

apply: ${{ github.event_name == 'workflow_dispatch' && inputs.apply == 'true' || github.event_name != 'workflow_dispatch' }}

New:

apply: ${{ github.event_name == 'workflow_dispatch' && inputs.apply == 'true' || github.event_name != 'workflow_dispatch' && vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY == 'true' }}

Previously scheduled/push runs always applied changes. Now they default to dry-run unless vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY is true. This is likely intentional (safer default), but it's a silent breaking change for repos that relied on the old behaviour. The changelog / PR description should call this out explicitly.

Code Quality

7. Redundant null-coalescing in EscapeCell (GitHubHousekeepingReportService.cs)

if (string.IsNullOrWhiteSpace(value))
    return "-";

var text = value ?? string.Empty;   // value cannot be null here

value is guaranteed non-null after the IsNullOrWhiteSpace early return. The ?? string.Empty is dead code. Use var text = value; or just inline value directly.

8. action.yml test assertion may be fragile (GitHubHousekeepingActionTests.cs line 510)

Assert.Contains("PowerForge GitHub Housekeeping", actionYaml, StringComparison.Ordinal);

"PowerForge GitHub Housekeeping" is the name of the reusable workflow (powerforge-github-housekeeping.yml), not of action.yml. If the composite action's metadata doesn't contain that exact string (e.g. it says "GitHub Housekeeping" or similar), this assertion will fail. Please verify the string actually appears in action.yml.

Minor / Docs

9. Minimal config dryRun field vs CLI flags

In the README minimal config example:

"dryRun": false

The actual dry-run/apply mode is controlled by the --dry-run / --apply CLI flag passed from the action. Having dryRun in the config as well could confuse users about which setting takes precedence. A short note clarifying this (or removing the field from the example) would help.

10. --output json no longer passed; step-summary now written by CLI

The architectural shift from the .ps1-parses-JSON model to the CLI-writes-files model is clean and the right direction. Just worth confirming in the PR description that GITHUB_STEP_SUMMARY is now populated by WriteGitHubHousekeepingOutputs directly, so callers that previously relied on the .ps1 step-summary format will see the new Markdown table format instead.

Summary

The path-fix, report service, and test structure are all good. The main items to address before merging are the $args variable name collision, the missing --token-env wire-up, and the wrong filename in the regression test assertion.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid set of changes. The core path-resolution bug fix (from ../.. to ../../..) is correct and migrating report writing from PowerShell into C# is the right architectural direction. A few issues worth addressing:

Bug: $args is a reserved PowerShell automatic variable

In action.yml, the Run GitHub housekeeping step assigns to $args as a local variable name. $args is a PowerShell automatic variable that holds unnamed parameters passed to a script or function. Assigning to it creates a local shadow that is fragile and can produce surprising behaviour in some contexts. Rename to $dotnetArgs or $cliArgs.

Behavioral breaking change: scheduled runs are now permanently dry-run

In github-housekeeping.yml the apply condition changed, and combined with the default apply input changing from 'true' to 'false', scheduled/push-triggered runs will never apply deletions unless vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY is explicitly set to 'true' in the repo. Previously they always applied. This silently disables active housekeeping for any repo relying on the schedule. Worth a callout in the README under migration notes.

Breaking change: workflow rename without deprecation path

Renaming reusable-github-housekeeping.yml to powerforge-github-housekeeping.yml breaks any consumer repo referencing the old name. There is no redirect or stub left behind. External repos pinned to @main will get a 404 on the old workflow ref. Consider leaving a stub with a deprecation comment pointing to the new name, or add a prominent migration note to the README.

Minor: BuildMarkdown is called redundantly

In Program.Command.GitHub.HousekeepingOutputs.cs, BuildMarkdown is always called eagerly even when only the JSON report path is set. If MarkdownReportPath is also set, BuildMarkdown runs twice because WriteMarkdownReport calls it internally too. A lazy evaluation pattern (e.g. string? md = null; string GetMd() => md ??= reports.BuildMarkdown(report);) avoids the redundant work.

Minor: Redundant null-coalescing in EscapeCell

In GitHubHousekeepingReportService.cs, after the IsNullOrWhiteSpace guard, value is guaranteed non-null, so var text = value ?? string.Empty can be simplified to just using value directly.

Observation: token env var rename may break existing configs

The action now sets GITHUB_TOKEN instead of POWERFORGE_GITHUB_TOKEN. Repos whose config still has "tokenEnvName": "POWERFORGE_GITHUB_TOKEN" will silently get no token and likely fail with auth errors. A migration note would help.

Positives

• if-no-files-found: ignore on the artifact upload avoids spurious failures when reporting is disabled.
• The regression test in GitHubHousekeepingActionTests that locks down the 3-level path resolution is excellent and will catch future regressions.
• Moving report generation from PowerShell into GitHubHousekeepingReportService is the right call: testable, consistent, and avoids duplicating formatting logic.
• FormatGiB using bit-shift (1L << 30) is clean.
• The collapsible detail tables with a 20-item cap and truncation notice is a nice UX touch.

Action items:

1. Rename $args to $dotnetArgs in action.yml (reserved automatic variable)
2. Document the scheduled-run behavior change and the POWERFORGE_GITHUB_HOUSEKEEPING_APPLY requirement
3. Add a deprecation stub or migration note for the renamed reusable workflow
4. (Optional) Lazy-evaluate BuildMarkdown to avoid the double call
5. (Cosmetic) Remove the redundant null-coalescing in EscapeCell

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid refactor — moving output generation into the CLI itself (instead of the PowerShell wrapper parsing JSON stdout) is cleaner and more testable. The path-depth fix (../../..) is clearly correct given the action lives three levels deep. A few issues worth addressing:

Bug: Token is no longer forwarded to the CLI

Critical. The old Invoke-PowerForgeHousekeeping.ps1 explicitly passed --token-env POWERFORGE_GITHUB_TOKEN to the CLI:

if (-not [string]::IsNullOrWhiteSpace($env:POWERFORGE_GITHUB_TOKEN)) {
    $null = $arguments.Add('--token-env')
    $null = $arguments.Add('POWERFORGE_GITHUB_TOKEN')
}

The new inline script sets GITHUB_TOKEN as an env var on the step but never passes --token-env GITHUB_TOKEN (or --token) to the dotnet run invocation. Unless the CLI auto-discovers GITHUB_TOKEN from the environment without being told which variable to read, API calls that require authentication will silently fail or use an unauthenticated client.

# action.yml - current (missing token arg)
$args = @('run', ... '--', 'github', 'housekeeping', '--config', $env:INPUT_CONFIG_PATH)
if ($env:INPUT_APPLY -eq 'true') { $args += '--apply' } else { $args += '--dry-run' }
& dotnet @args

Please verify the CLI behavior and add '--token-env', 'GITHUB_TOKEN' if needed.

Bug: $args is a PowerShell automatic variable

In the inline run: script, $args is assigned as a local array:

$args = @('run', '--project', $project, ...)

$args is an automatic variable that holds unbound positional parameters. Reassigning it at script scope works on some PS versions but is confusing and could behave unexpectedly in some edge cases. Rename to $cliArgs or $arguments.

Missing config-path validation

The old script validated the resolved config path before invoking the CLI:

if (-not (Test-Path -LiteralPath $configPath)) {
    throw "Housekeeping config not found: $configPath"
}

The new script passes $env:INPUT_CONFIG_PATH directly without checking whether it's empty or the file exists. A missing or empty config will produce a less actionable CLI error. Consider re-adding the guard, especially since an empty INPUT_CONFIG_PATH becomes an empty --config argument.

Minor: Redundant null-coalescing in EscapeCell

In GitHubHousekeepingReportService.cs:

if (string.IsNullOrWhiteSpace(value))
    return "-";

var text = value ?? string.Empty; // value is guaranteed non-null here

string.IsNullOrWhiteSpace returns true for null, so the ?? string.Empty on the next line is dead code. Use value! or just value (with a ! suppressor or a #pragma).

Minor: Duplicated UTF-8 file-write logic

AppendUtf8 in Program.Command.GitHub.HousekeepingOutputs.cs and WriteUtf8 in GitHubHousekeepingReportService.cs both implement the same "create directory then write file" pattern. The difference (AppendAllText vs WriteAllText) is intentional for GITHUB_OUTPUT/GITHUB_STEP_SUMMARY, so a shared helper isn't straightforward — but worth a comment explaining why the two exist.

Behavior change: scheduled runs now default to dry-run

Old trigger logic in github-housekeeping.yml:

apply: ${{ github.event_name == 'workflow_dispatch' && inputs.apply == 'true' || github.event_name != 'workflow_dispatch' }}

Scheduled runs would always apply deletions.

New logic:

apply: ${{ (github.event_name == 'workflow_dispatch' && inputs.apply == 'true') || (github.event_name != 'workflow_dispatch' && vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY == 'true') }}

Scheduled runs are now dry-run unless POWERFORGE_GITHUB_HOUSEKEEPING_APPLY is explicitly set to 'true'. This is safer as a default but is a breaking change for existing users relying on automatic cleanup via the schedule trigger. Worth calling out explicitly in the PR description/CHANGELOG.

Observation: brittle string-match tests in GitHubHousekeepingActionTests

Tests like:

Assert.Contains("../../../global.json", actionYaml, StringComparison.Ordinal);
Assert.Contains("PowerForge GitHub Housekeeping", actionYaml, StringComparison.Ordinal);
Assert.DoesNotContain("Invoke-GitHubHousekeeping.ps1", actionYaml, StringComparison.Ordinal);

These are regression guards against the exact bug being fixed, which is reasonable. However they are coupled to YAML formatting details. If the step description or path appears in a comment with different spacing the test could give a false positive/negative. Consider parsing the YAML and asserting on the structured values instead, or at least adding a comment explaining the intent.

Summary

The report service and model are clean and well-structured. The test coverage for GitHubHousekeepingReportServiceTests is solid. Please address the token-forwarding issue before merging.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a well-structured fix with good test coverage. The core path-depth correction (../.. → ../../..) is clearly correct, and the new reporting layer is a clean addition. A few issues worth addressing before merging:

Bug: $args is a reserved PowerShell automatic variable

action.yml – "Run GitHub housekeeping" step (the inlined script):

$args = @('run', '--project', $project, '-c', 'Release', '--no-build', '--framework', 'net10.0', '--', ...)
& dotnet @args

$args is a PowerShell automatic variable that holds the unamed parameters passed to a script or function. Assigning to it in script scope can produce unexpected behavior — in some contexts the assignment silently succeeds but the value is read-only, and in others later code may overwrite it. The deleted Invoke-PowerForgeHousekeeping.ps1 correctly used $arguments for this. Renaming back to $arguments (or any non-reserved name) avoids the risk entirely.

Potential regression: token no longer forwarded to the CLI

The old script explicitly forwarded the token with --token-env POWERFORGE_GITHUB_TOKEN. The new inline script drops that flag entirely and only passes GITHUB_TOKEN as an env var:

env:
  GITHUB_TOKEN: ${{ inputs['github-token'] != '' && inputs['github-token'] || github.token }}

This works if and only if the CLI reads GITHUB_TOKEN from the environment by default (which it likely does since this is the standard GitHub Actions env var). However, this is a silent behaviour change from the previous explicit --token-env wiring. Please confirm the CLI falls back to GITHUB_TOKEN automatically, and if so, a brief comment in the action or CLI code noting the convention would help future maintainers.

Minor: dead-code null guard in EscapeCell

GitHubHousekeepingReportService.cs line ~992–997:

if (string.IsNullOrWhiteSpace(value))
    return "-";

var text = value ?? string.Empty;   // ← value is guaranteed non-null here

IsNullOrWhiteSpace already handles null, so the ?? string.Empty fallback can never trigger. Use var text = value; (or just inline it in the .Replace chain).

Behaviour change in github-housekeeping.yml (worth documenting)

The apply default changed from true to false, and the scheduled-run condition changed from:

# old: auto-applies on any non-dispatch trigger
apply: ${{ github.event_name == 'workflow_dispatch' && inputs.apply == 'true' || github.event_name != 'workflow_dispatch' }}

to:

# new: requires an explicit repo variable to apply on schedule
apply: ${{ ... || (github.event_name != 'workflow_dispatch' && vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY == 'true') }}

This is a safer default, but it is a breaking change for any repo that relies on the scheduled workflow applying deletions automatically. Consider a release note or a migration note in the README so callers know they must set POWERFORGE_GITHUB_HOUSEKEEPING_APPLY=true as a repo/org variable to restore the old behaviour.

Hardcoded --framework net10.0

$args = @('run', ..., '--framework', 'net10.0', ...)

This is fine if global.json always pins .NET 10, but it will silently break if the framework target ever changes or if a runner doesn't have the .NET 10 SDK installed yet. Driving this from a variable or deriving it from global.json would be more robust long-term.

Test observations

GitHubHousekeepingActionTests.CompositeAction_AssetPaths_ShouldResolveFromActionDirectory is a solid regression guard for the path-depth bug. One small nit: Assert.Contains("PowerForge GitHub Housekeeping", actionYaml) checks a display name that lives in powerforge-github-housekeeping.yml, not in action.yml — so if this assertion ever fails it could be confusing. The test reads action.yml but the string only appears in the workflow file. Consider splitting into separate assertions with clear messages.

Summary

The path-depth fix and the new reporting layer are both clean. Addressing the $args rename and confirming the token flow would make this merge-ready.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid, well-motivated refactor. The core path fix is correct and the new GitHubHousekeepingReportService is a clean abstraction. A few issues worth addressing before merging:

🔴 Potential Breaking Change — Scheduled runs may silently stop cleaning up

.github/workflows/github-housekeeping.yml

The apply condition changed from:

# Old: non-dispatch events always applied
apply: ${{ github.event_name == 'workflow_dispatch' && inputs.apply == 'true' || github.event_name != 'workflow_dispatch' }}

to:

# New: non-dispatch events require a repo variable
apply: ${{ (github.event_name == 'workflow_dispatch' && inputs.apply == 'true') || (github.event_name != 'workflow_dispatch' && vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY == 'true') }}

Any repo running this on a schedule (cron) will now silently enter dry-run mode until POWERFORGE_GITHUB_HOUSEKEEPING_APPLY is set as a repository variable. This should be prominently noted in the PR description and ideally in the README. Callers using the old reusable-github-housekeeping.yml@main reference will get this change silently when the file is renamed.

🟡 Missing actions: write permission in runner housekeeping workflow

.github/workflows/powerforge-github-runner-housekeeping.yml line 460

permissions:
  contents: read

The composite action (github-housekeeping) can delete caches and artifacts, which requires actions: write. If a caller's runner-housekeeping.json enables caches or artifacts, this will fail with a permission error at runtime. Compare to powerforge-github-housekeeping.yml which correctly declares actions: write. Either document that runner-only configs must not enable those sections, or add the permission here.

🟡 Token env-var rename could interact with Actions' built-in GITHUB_TOKEN

action.yml line 324

-        POWERFORGE_GITHUB_TOKEN: ${{ inputs['github-token'] != '' && inputs['github-token'] || github.token }}
+        GITHUB_TOKEN: ${{ inputs['github-token'] != '' && inputs['github-token'] || github.token }}

Renaming to GITHUB_TOKEN means the step's env overrides the automatically injected GITHUB_TOKEN from the Actions runner. This works, but any future composite steps nested inside this action that rely on the built-in GITHUB_TOKEN could pick up a PAT inadvertently. A more scoped env-var name (e.g. POWERFORGE_GITHUB_TOKEN) avoids this ambiguity. If the CLI is being changed to read GITHUB_TOKEN, consider whether that's the right long-term interface.

🟡 Fragile YAML-content tests

PowerForge.Tests/GitHubHousekeepingActionTests.cs lines 628–636

Assert.Contains("$dotnetArgs", actionYaml, StringComparison.Ordinal);
Assert.Contains("Housekeeping config not found", actionYaml, StringComparison.Ordinal);
Assert.Contains("PowerForge GitHub Housekeeping", actionYaml, StringComparison.Ordinal);

Asserting on raw YAML/PS source strings couples these tests tightly to implementation details rather than behaviour. Renaming a variable or rewording an error message breaks the tests without any functional regression. The path-resolution tests (global.json, PowerForge.Cli.csproj exist at ../../..) are valuable; the source-string assertions are not. Consider removing or replacing them with assertions on what the action produces.

🟢 Minor — Redundant null-coalescing in EscapeCell

PowerForge/Services/GitHubHousekeepingReportService.cs line 1152

if (string.IsNullOrWhiteSpace(value))
    return "-";

var text = value ?? string.Empty;   // value is guaranteed non-null here

After the IsNullOrWhiteSpace guard, value is always non-null. value ?? string.Empty can be replaced with just value! or value (with a ! suppression if needed), or the guard can be written as if (value is null or { Length: 0 }) and value can be used directly after.

✅ What's well done

• Core path fix (../.. → ../../..) is correct and the regression test that verifies the paths resolve to real files is exactly the right way to guard against this class of bug.
• Inlining the PS script into action.yml reduces the surface area and removes a stale file; the typed $dotnetArgs array is cleaner than the old List<string>.
• Moving summary/report generation into C# (GitHubHousekeepingReportService) is the right architectural move — it keeps the formatting logic testable and version-controlled alongside the service models.
• if-no-files-found: ignore on the upload step is the correct choice; a dry run that produces no report shouldn't fail the workflow.
• Defaulting the manual dispatch apply input to 'false' is a safer default.

Summary: The path fix and report-service additions are good. Please address the silent behaviour change for scheduled runs (#1) and the missing permission in the runner workflow (#2) before merging.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

PR Review: Fix reusable GitHub housekeeping action

Overall this is a well-structured refactor. The core path fix (from ../..​ to ../../..​) is correct, the move to inlined PowerShell removes a deployment dependency, and the new C# report service is cleanly separated. A few items worth looking at before merging:

Potential Bug: Token not passed to CLI

The old script passed --token-env POWERFORGE_GITHUB_TOKEN to the CLI only when that env var was set. The new inlined script drops this entirely and only sets GITHUB_TOKEN in the environment — no --token or --token-env argument is appended to $dotnetArgs. If the CLI does not automatically fall back to reading GITHUB_TOKEN, API calls will fail with auth errors. Worth confirming the CLI handles this automatically.

Behavioral Change: Scheduled runs now default to dry-run

github-housekeeping.yml changed scheduled runs from always applying to requiring vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY == 'true'. This is a safe default, but any repo using the scheduled trigger without setting that variable will silently switch to dry-run only. Worth noting in migration docs.

Minor: Redundant null-coalescing in EscapeCell

In GitHubHousekeepingReportService.cs, after the IsNullOrWhiteSpace guard, value is provably non-null. The value ?? string.Empty line is unreachable and can be simplified to just value.

Duplication: AppendUtf8 vs WriteUtf8

Program.Command.GitHub.HousekeepingOutputs.cs has AppendUtf8 and GitHubHousekeepingReportService.cs has WriteUtf8 — both create the parent directory and write UTF-8-without-BOM content. The only difference is append vs overwrite. Consider a shared helper to avoid divergence.

Breaking Change: Workflow rename

Renaming reusable-github-housekeeping.yml to powerforge-github-housekeeping.yml breaks external callers. The README is updated, but there is no compatibility stub or redirect. External repos pinned to the old name will fail. Consider a forwarding stub or a prominent deprecation notice.

Hardcoded framework version in action.yml

--framework net10.0 is hardcoded in the dotnet run args. If the project ever changes target frameworks this will break. If the project is single-targeted, the flag is redundant; if multi-targeted, deriving the value from global.json or the project file would be more resilient.

Test quality: Action test checks hardcoded strings

GitHubHousekeepingActionTests asserts for specific literals like "$dotnetArgs" and "Housekeeping config not found" inside action.yml. These are reasonable regression guards for the path-fix, but a short comment explaining their intent would help future maintainers avoid removing them unnecessarily.

Security: Report path traversal (low risk)

GetGitHubHousekeepingOutputOptions resolves POWERFORGE_GITHUB_HOUSEKEEPING_REPORT_PATH without verifying the resolved path stays within the workspace. Path.GetFullPath normalizes traversal sequences but does not constrain the output. Since these values ultimately come from caller-supplied inputs, consider clamping to GITHUB_WORKSPACE.

The core fixes (path resolution, inlined PS, typed arg list, report artifacts, new runner workflow) look correct and are a clear improvement. The token-passing change is the item I would confirm before merging.

Code Review: Fix reusable GitHub housekeeping action

Overall this is a clean, well-structured refactor. Moving JSON parsing and report generation from the PowerShell wrapper into the C# CLI is a better long-term design. Here are some observations:

Potential bugs / functional regressions

1. Token is no longer passed to the CLI (action.yml - Run GitHub housekeeping step)

The old Invoke-PowerForgeHousekeeping.ps1 explicitly added --token-env POWERFORGE_GITHUB_TOKEN to the CLI args. The new inlined script omits any --token-env argument and renames the env var to GITHUB_TOKEN. Unless the CLI was updated to auto-detect GITHUB_TOKEN (not visible in this diff), the housekeeping command may fail to authenticate. Worth confirming this is intentional and the CLI picks up the new env var name.

2. --output json flag is no longer passed

The old script always appended --output json so it could parse the result envelope. The new script omits it. If the CLI console output is now irrelevant since reports go to files/GITHUB_OUTPUT, this is fine, but any callers consuming stdout JSON will silently break.

3. powerforge-github-runner-housekeeping.yml missing actions: write

The runner housekeeping workflow only declares contents: read. The shared composite action can also manage caches and artifacts; if a caller config accidentally enables those sections, the job will fail with permission errors at runtime. Consider documenting that actions: write is required when cache or artifact sections are enabled.

Breaking change worth calling out

4. Scheduled runs no longer apply by default (github-housekeeping.yml)

Old condition: apply when event is not workflow_dispatch (scheduled runs always applied). New condition: scheduled runs only apply when vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY is true. Any repository relying on scheduled cleanup will silently stop deleting after merging this PR unless that variable is set. A migration note would prevent surprise.

Minor code quality observations

5. EscapeCell CRLF handling (GitHubHousekeepingReportService.cs)

The method replaces carriage-return with a space and newline with br. A CRLF sequence becomes space+br because CR is replaced first. Replacing CRLF as a unit before individual replacements would be cleaner, but this is cosmetic.

6. StorageStatus parameter name vs intent

The parameter named eligible is passed PlannedDeletes at the call sites. The name mismatch makes the method signature harder to read; plannedDeletes would match callers.

7. Fragile snapshot assertions in GitHubHousekeepingActionTests.cs

Assertions testing literal YAML text will break the test on any formatting change to action.yml without any functional regression. The path-resolution assertions (verifying resolved files actually exist on disk) are genuinely useful; the string-contains ones less so.

What is well done

• The depth-3 path fix (../../..) is correct and the regression test validating it is a solid addition.
• Moving report generation into the C# layer is less fragile than shell-side JSON parsing.
• The lazy GetMarkdown() local function in WriteGitHubHousekeepingOutputs avoids rendering markdown twice when both paths are set.
• Collapsible details sections in the Markdown report with a truncation notice at 20 items are a nice UX touch.
• UTF8Encoding without BOM throughout is correct for GitHub Actions files.

The core bug fix and new report/artifact plumbing look solid. The main things worth addressing before merge are the token passthrough question (point 1) and the silent dry-run regression for scheduled runs (point 4).

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

PR Review: Fix reusable GitHub housekeeping action

Overall this is a solid, well-motivated PR. The core path-resolution bug is real and the fix is correct. The new C# report service is clean and well-tested. A few items worth addressing before merging:

Bug fix correctness ✅

The root cause is correctly identified and fixed. The composite action lives at .github/actions/github-housekeeping/, so the repo root is three levels up, not two:

.github/actions/github-housekeeping  →  (../../../)  →  repo root

The old ../.. resolved to .github/ — definitely broken. Both the global-json-file path and the $repoRoot resolution in the build/run steps now correctly use ../../... Good.

Issues

1. Breaking behavior change: config-path no longer has a fallback default

The old Invoke-PowerForgeHousekeeping.ps1 defaulted INPUT_CONFIG_PATH to .powerforge/github-housekeeping.json when empty. The new inlined script throws instead:

if ([string]::IsNullOrWhiteSpace($configPath)) {
  throw "Housekeeping config path is required."
}

Any caller that invokes the composite action without an explicit config-path input will now get a hard failure. If this is intentional (forcing explicit config), it should be noted in the PR description and ideally the action.yml input should have required: true rather than required: false with a default path that's never actually used.

2. Hardcoded TFM is a future maintenance risk

$dotnetArgs = @('run', '--project', $project, '-c', 'Release', '--no-build', '--framework', 'net10.0', ...)

The comment # Keep this framework in sync with the runnable TFM in PowerForge.Cli.csproj acknowledges the coupling. When the project upgrades to net11.0 or adds a new TFM, this string won't get updated by the normal tooling. Consider driving this from the csproj (e.g., reading the <TargetFramework> tag) or at minimum adding a CI check / test that validates the TFM here matches what's in the csproj.

3. Silent behavior change for scheduled runs

github-housekeeping.yml changed the apply condition from:

# before: always apply on non-dispatch events (e.g., schedule)
apply: ${{ github.event_name == 'workflow_dispatch' && inputs.apply == 'true' || github.event_name != 'workflow_dispatch' }}

to:

# after: only apply on non-dispatch events if POWERFORGE_GITHUB_HOUSEKEEPING_APPLY var is set
apply: ${{ (github.event_name == 'workflow_dispatch' && inputs.apply == 'true') || (github.event_name != 'workflow_dispatch' && vars.POWERFORGE_GITHUB_HOUSEKEEPING_APPLY == 'true') }}

Existing consumers with scheduled runs will silently switch to dry-run mode until they set the repo variable. This should be called out prominently in release notes or a migration note.

4. apply type inconsistency across workflows

powerforge-github-runner-housekeeping.yml declares apply as type: boolean with default: true, while powerforge-github-housekeeping.yml uses string inputs. GitHub Actions coerces these, but it's an inconsistency that could confuse callers and produce surprising results with the inputs.apply && 'true' || 'false' ternary.

Minor observations

Unused overload — WriteMarkdownReport(string path, GitHubHousekeepingReport report) in GitHubHousekeepingReportService appears to be dead code; the call site in HousekeepingOutputs.cs always uses the (string path, string markdown) overload via GetMarkdown(). Safe to remove.

Test brittleness — The YAML-string-grep tests (GitHubHousekeepingActionTests, GitHubRunnerHousekeepingWorkflowTests) are useful regression guards but will fail on any formatting change to the YAML even when behavior is unchanged. Consider wrapping the most fragile assertions (e.g., Assert.Contains("$dotnetArgs", actionYaml)) in a comment explaining what invariant they're actually protecting.

EscapeCell null-forgiving — var text = value!; on line ~1202 is reachable only after the IsNullOrWhiteSpace guard so the ! is unnecessary. Not a bug, just noise.

Summary

The first two ⚠️ items are worth resolving or explicitly documenting before merge.